-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MU4] [WIP] GSoC 2020: Albums #6145
Conversation
libmscore/layout.cpp
Outdated
if (!page || curPage >= score->npages()) { | ||
page = new Page(score); | ||
score->pages().push_back(page); | ||
if (!page || curPage >= dominantScore->npages()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is based on master
, better to start using the new coding standard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :-)
libmscore/layout.cpp
Outdated
// append systems to the page until you find a page break | ||
// or run out of Movements (MasterScores) | ||
// | ||
// called after LayoutContext::getNextPage() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to see this kind of comments! Makes it more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments are so good that you should prefix them with ///
(three slashes) to expose them to Doxygen. Don't use three slashes before the function name (// collectPage
) or lines with dashes (//-----------
).
libmscore/layout.cpp
Outdated
} | ||
if (breakPage) { | ||
qreal dist = qMax(prevSystem->minBottom(), prevSystem->spacerDistance(false)); | ||
dist = qMax(dist, slb); | ||
layoutPage(page, ey - (y + dist)); | ||
// if we collected a system we cannot fit onto this page, | ||
// we need to collect next page in order to correctly set system positions | ||
if (collected) | ||
if (collected) { // έχει γίνει κατάχρηση της collected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my Greek is not good enough to read this ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆 the comments in Greek are my notes, they will be removed. This one says that I've abused collected
.
libmscore/layout.cpp
Outdated
@@ -4579,14 +4619,18 @@ void Score::doLayoutRange(const Fraction& st, const Fraction& et) | |||
void LayoutContext::layout() | |||
{ | |||
MeasureBase* lmb; | |||
dominantScore = static_cast<MasterScore*>(score); // set the first score (masterscore = movement) as the dominant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you use toScore(score)
instead of this cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toScore
seems to return a Score*
not a MasterScore*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And there is no toMasterScore()
(unless you add it in libmscore/scoreElement.h
😉). In general we try to prevent these kind of casts.
@@ -0,0 +1,55 @@ | |||
#include "albummanagerdialog.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Standard header missing (copyright etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I will add it :-)
@@ -0,0 +1,29 @@ | |||
#ifndef ALBUMMANAGERDIALOG_H |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Standard header missing.
mscore/file.cpp
Outdated
@@ -298,7 +299,8 @@ void MuseScore::openFiles(bool switchTab, bool singleFile) | |||
<< tr("Bagpipe Music Writer Files (experimental)") + " (*.bww)" | |||
<< tr("Guitar Pro Files") + " (*.gtp *.gp3 *.gp4 *.gp5 *.gpx *.gp)" | |||
<< tr("Power Tab Editor Files (experimental)") + " (*.ptb)" | |||
<< tr("MuseScore Backup Files") + " (*.mscz, *.mscx,)"; | |||
<< tr("MuseScore Backup Files") + " (*.mscz, *.mscx,)" | |||
<< tr("MuseScore Album Files") + " (*.msca *.album)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are both .msca
and .album
identical? Or is one of them a compressed file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.msca
is the new albums file.
.album
is the MuseScore2 album type.
They are same-ish, the new one has more information and is (in my opinion) more fitting. .album
could be anything.
mscore/revision.h
Outdated
@@ -1 +1 @@ | |||
3543170 | |||
e122e7c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file supposed to check-in???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops, I'll remove that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better install that hook to prevent this. Check https://musescore.org/en/handbook/developers-handbook/finding-your-way-around/git-workflow#Summary step 4:
In your clone directory, copy the file build/git/hooks/post-checkout to the directory .git/hooks in order for mscore/revision.h to be maintained automatically by git. See build/git/hooks/README for details. Note that the .git directory is hidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll do that tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Do I need to squash my commits for this to take effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think so
<name>My Album</name> | ||
<Score> | ||
<alias>Piano1</alias> | ||
<path>/home/boop/Documents/GSoC_2020/mscore/MuseScore/mtest/mscore/albums/Piano1.mscx</path> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having these absolute path makes it difficult to transfer albums.
Is it planned to have albums as compressed folders, containing all scores etc., just like the mscz file is a compressed folder containing the score plus some other files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am planning to have both absolute and relative paths (if one does not work, the other might save the day).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a compressed file containing all the score of the album, together with the new msca
file? But that can be done after the albums itself are finalized.
3a52d10
to
e8edc65
Compare
Once more a rebase seems needed. And the Travis fails is not fixed, see https://travis-ci.org/github/musescore/MuseScore/jobs/692713789#L3927-L3929, it is missing ui_albummanager.h |
mscore/albummanager.h
Outdated
QTableWidgetItem* listItem { nullptr }; | ||
QTableWidgetItem* listDurationItem { nullptr }; | ||
QTableWidgetItem* listItem { nullptr }; // αυτά δεν πρεπει να ειναι εδω | ||
QTableWidgetItem* listDurationItem { nullptr }; // και αυτο |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I can understand that this a) is Work In Progress and b) Greek is your native language and as such more comfortable to you, I'd still prefer comments in English, as soon as it gets published here on GitHub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take care of that tomorrow :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Now there are linker errors and some compiler warnings too on AppVeyor and mtest fails on Travis |
rebase needed... |
Rebased and updated relative file path code. |
mscore/albummanager.cpp
Outdated
tempScore = _items.at(0)->albumItem->score->clone(); // TODO: clone breaks editing sync for the 1st movement | ||
tempScore = _items.at(0)->albumItem->score->clone(); // clone breaks editing sync for the 1st movement | ||
while (tempScore->systems().size() > 1) { // remove the measures of the cloned masterscore, that way editing is synced | ||
for(auto x : tempScore->systems().last()->measures()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space between for
and (
libmscore/album.cpp
Outdated
//--------------------------------------------------------- | ||
//--------------------------------------------------------- | ||
// | ||
// AlbumItem | ||
// | ||
//--------------------------------------------------------- | ||
//--------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need a comment chunk so large...
//---------------------------------------------------------
// AlbumItem
//---------------------------------------------------------
is good already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it isn't too much of a problem, I would like to keep it like that until I the PR is ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
libmscore/album.cpp
Outdated
|
||
AlbumItem::AlbumItem(Album* album) | ||
{ | ||
this->album = album; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make class variables prefixed with m_
, as stated in the new code guideline. Then we don't need this->
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, I'll take care of it
libmscore/album.cpp
Outdated
this->album = album; | ||
} | ||
|
||
AlbumItem::AlbumItem(Album* album, MasterScore *score, bool enabled) : AlbumItem(album) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's best to take : AlbumItem(album)
down a new line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I have ignored the line width limits in multiple places. I will take care of that in my next refactor.
libmscore/album.cpp
Outdated
|
||
AlbumItem::AlbumItem(Album* album, MasterScore *score, bool enabled) : AlbumItem(album) | ||
{ | ||
std::cout << "New album item..." << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use qDebug
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will probably delete it when I reach an acceptable level of stability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use qDebug
you probably won't have to delete them :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm no, final qDebug
s don't need these details. So fine, you can use std::cout
s and delete them when they aren't needed anymore.
Ignore the latest commits, I am reworking them (pushed to see the changes in a better interface). |
21a84a4
to
5bc6443
Compare
In this file I will be writing down bugs, issues and thoughts so that both you and I can see/remember what I am working on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some very promising work here! I have a few comments. I've tried to keep style stuff to a minimum, but there's still a few things I thought I should pick up. But really, this is great work! Keep it up 🚀
libmscore/album.cpp
Outdated
void Album::addScore(MasterScore *score, bool enabled) | ||
{ | ||
if (!score) { | ||
std::cout << "There is no score to add to album..." << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We tend to use qDebug
for messages like this, which also lets us know where the message is coming from if it appears in console, iirc.
edit: although I see Howard has already mentioned this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove that soon enough.
libmscore/album.cpp
Outdated
return nullptr; | ||
} | ||
|
||
MasterScore* Album::removeScore(int index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have this return a MasterScore
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason at all, a mistake that was causing me trouble. I'll change that.
libmscore/album.cpp
Outdated
// getDominant | ||
//--------------------------------------------------------- | ||
|
||
MasterScore *Album::getDominant() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style :)
MasterScore *
-> MasterScore*
libmscore/album.cpp
Outdated
// loadFromFile | ||
//--------------------------------------------------------- | ||
|
||
bool Ms::Album::loadFromFile(const QString &path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style, again:
const QString &path
-> const QString& path
I won't mention any more things like this, because I think running uncrustify should be enough to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think so too, those were from before I found out how to edit my QtCreator settings :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And where did you find that out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Marr11317 QtCreator Menubar -> Tools -> Options -> C++, then:
- Select the built-in Qt style.
- Press
Copy...
(because you can't edit the built-in style. - Press
Edit...
- Go to
Pointers and References
. - Deselect
Identifier
and selectType name
(and maybe left const/volatile?).
libmscore/score.h
Outdated
virtual bool isMaster() const override { return true; } | ||
virtual ElementType type() const override { return ElementType::SCORE; } // TODO: if I change it to MasterScore scores are not drawn in album-mode | ||
|
||
virtual bool isMaster() const override { return true; } // TODO: should this be removed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: should this be removed?
Probably not! I get 60 results across the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be replaced by is MasterScore
(and probably will be in the future if those classes are rewritten) but as of right now a MasterScore
needs to have ElementType::SCORE
(otherwise nothing gets drawn on the screenview). So right now toMasterScore/isMasterScore
are useless.
mscore/albummanager.h
Outdated
Q_OBJECT | ||
|
||
public: | ||
// can I convert these to references? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, will they ever have a nullptr
value as pointers? If the answer to that question is 'yes', then the answer is no, you can't convert these to references. That's simply because there's no such thing as a null reference.
mscore/albummanagerdialog.cpp
Outdated
break; | ||
case QDialogButtonBox::Ok: | ||
apply(); | ||
// intentional ?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems so. 'OK' has the behaviour of hiding the window, whereas 'Apply' does not, by convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just copy pasted that (comment included) from the prefsDialog.cpp
.
midi.setMinChunkSize(10); | ||
|
||
if (!heartBeatTimer->isActive()) { | ||
heartBeatTimer->start(20); // msec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this magic number come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy-paste from Seq::setScoreView
.
} | ||
|
||
midi = MidiRenderer(cs); | ||
midi.setMinChunkSize(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another magic number?
<name>My Album</name> | ||
<Score> | ||
<alias></alias> | ||
<path>/home/boop/Documents/GSoC_2020/mscore/MuseScore/mtest/mscore/albums/Piano1.mscx</path> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will /home/boop
work when this is being run by Travis? I'd guess no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, fixing the paths of the tests is on my ToDo list :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to avoid absolute paths where possible to avoid leaking information about the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially, in this case, absolute paths would need a very smart workaround to not fail. I was thinking to give the users the ability to select whether they want both relative and absolute paths when saving/exporting.
Travis still doesn't like it. Here it shows yellow, in progress, but really it is read, failed, and in the mtests. Several address sanitizer failures, but also other, including tst_albums |
4 of the failures are basically the same problem. I've been working on that for a couple of days (I believe that I will solve this today). The tst_albums one is probably a simple fix and I will address it together with @shoogle's and @jthistle's feedback about absolute paths (works locally). The tst_tools failure is not present locally so that will be the last one that I tackle. |
Only the album test remains :^) |
mscore/albummanager.h
Outdated
void currentScoreChanged(int); | ||
void itemChanged(QListWidgetItem*); // score name in list is edited | ||
void buttonBoxClicked(QAbstractButton*); | ||
void addClicked(bool throwaway = false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to give these throwaway
parameters a name. This is valid syntax:
void addClicked(bool = false);
Doing this avoids compiler warnings that appear when you give something a name and never actually use it.
See this comment for an alterative solution using Q_UNUSED(throwaway)
in the .cpp
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in the latest push.
Front-cover related code is hard-coded (for now) for demonstration purposes. |
I fixed the inspector, hope I didn't break anything :-) |
Some comments after having tested this: https://gist.github.com/jthistle/1cffc1393ecddd2841e1f5424340c5a6 All in all, this works very well so far! I was genuinely surprised by how functional it already is. Obviously there are bugs and problems, but that's completely normal, and you have ample time to fix them. |
mscore/albummanager.cpp
Outdated
@@ -238,17 +241,11 @@ void AlbumManager::albumNameChanged(const QString& text) | |||
t->setPlainText(""); | |||
} else if (t->tid() == Tid::COMPOSER) { | |||
t->setSize(16); | |||
t->setPlainText("Sergios - Anestis Kefalidis\nJames Thistlewood\nJoachim Schmitz"); | |||
// t->setAlign(Align::CENTER); | |||
t->setPlainText(m_album->composers().join("\n")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, might better be a comma separated list
mscore/albummanager.cpp
Outdated
} else if (t->tid() == Tid::POET) { | ||
t->setSize(16); | ||
// t->setAlign(Align::CENTER); | ||
t->setPlainText(m_album->lyricists().join("\n")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
…nabling/disabling for parts, various other improvements
Added tabstops and cleaned ui file, again
Albums will be reimplemented for 4.x. This PR cannot be merged because it is based on 3.x code, which no longer exists |
Resolves: Albums missing :-)